Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cli/sql: new parameter --watch to repeat the -e statements #40594

Merged
merged 1 commit into from
Sep 19, 2019
Merged

cli/sql: new parameter --watch to repeat the -e statements #40594

merged 1 commit into from
Sep 19, 2019

Conversation

knz
Copy link
Contributor

@knz knz commented Sep 9, 2019 •
edited
Loading

Release justification: needed in tests

I found myself needing this for scenario-based testing.
The importance of this compared to enclosing cockroach sql in a shell loop is that I want/need to avoid the overhead of establishing a new SQL connection each time.

It would greatly simplify my workflow to have this on master ASAP, but if the TL in charge thinks "too late" I'll accept (a little reluctantly) delaying until the branch is cut.

@knz knz requested review from maddyblue and bdarnell September 9, 2019 10:47
@knz knz requested a review from a team as a code owner September 9, 2019 10:47
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz
cli/sql: new parameter --watch to repeat the -e statements
dea2a03 
Release note (cli change): `cockroach sql` now supports the `--watch`
command line parameter. When used to specified a duration, the client
will repeat the statement(s) specified by `-e` until an error occurs.
This is intended for simple monitoring scenarios during development
and testing.
@knz knz force-pushed the 20190909-cli-watch branch from 6a5f9bf to dea2a03 Compare September 9, 2019 10:49
maddyblue
maddyblue approved these changes Sep 9, 2019
View reviewed changes
Copy link
Contributor

@maddyblue maddyblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM.

However I am forced to compare this to my own PR #40453 where I also did things to our cli in order to assist in my own testing. You were opposed to that, suggesting I write a custom command to do it. Is there a reason this PR should be accepted and mine should not? To me both appear to be "let's make something a bit weirder so we can test better".

@knz
Copy link
Contributor Author

knz commented Sep 9, 2019 via email

@maddyblue
Copy link
Contributor

I think you should just go ahead and merge this PR because it's useful.

But I'm not convinced that our line of merging things that are useful for testing should be at the interactive shell. Seems kinda arbitrary. Maybe it should? Maybe my PR could solve its problem in a different way that doesn't involve changing anything on the interactive side?

@rohany rohany mentioned this pull request Sep 9, 2019
Merged
@knz knz mentioned this pull request Sep 10, 2019
Merged
@knz
Copy link
Contributor Author

knz commented Sep 10, 2019 •
edited
Loading

our line of merging things that are useful for testing should be at the interactive shell. Seems kinda arbitrary.

I think my line of thinking remains the following:

  • the two things optimize for different goals that are contradictory with each other. testing tools need to optimize for regularity, a fixed format over time, a format easy to parse, which means simple, compact and noise-free. An interactive tool (especially demo which is used to showcase things) needs to optimize for clarity, change over time as the interests of users evolve, be verbose and explanatory.

    Trying to unify the two is a tough challenge! Even if senior engineers like you can square that circle for an incidental assignment, it seems unlikely the next person who may not have that experience will be able to reconcile the goals.

  • there is an argument about "locality of change". Today contributors to the SQL shell have just one goal to satisfy and that is UX for interactive users. They have just one "audience" and it makes the tool rather self-contained. If we start to reuse the tool for other purposes that extends the audience, and makes it harder for contributors to get things done - they have more folk to talk to and compromise with.

  • finally another thought I had forgotten earlier: we have an ongoing issue with readline and @bdarnell and I were mulling over the possibility to split cockroach sql (and thus demo) into a separate project that would be GPL-compatible, so we can use GNU readline.

@jordanlewis
Copy link
Member

Not really weighing in here but couldn't you use the Linux watch tool for this?

@knz
Copy link
Contributor Author

knz commented Sep 10, 2019 •
edited
Loading

@jordanlewis psql's own \watch is inspired from the venerable unix watch. The reason why folk use \watch and why I am introducing --watch here is the same: to avoid the overhead of reconnecting to the db each time. I am looking for sub-second updates and the SQL connection handshake eats a lot of a second already.

@knz
Copy link
Contributor Author

knz commented Sep 19, 2019

bors r+

@craig
Copy link
Contributor

craig bot commented Sep 19, 2019

Build failed (retrying...)

craig bot pushed a commit that referenced this pull request Sep 19, 2019
@knz
Merge #40594
a8c4dd0 
40594: cli/sql: new parameter --watch to repeat the -e statements r=knz a=knz

Release justification: needed in tests

I found myself needing this for scenario-based testing.
The importance of this compared to enclosing `cockroach sql` in a shell loop is that I want/need to avoid the overhead of establishing a new SQL connection each time.

It would greatly simplify my workflow to have this on `master` ASAP, but if the TL in charge thinks "too late" I'll accept (a little reluctantly) delaying until the branch is cut.

Co-authored-by: Raphael 'kena' Poss <[email protected]>
@craig
Copy link
Contributor

craig bot commented Sep 19, 2019

Build succeeded

@craig craig bot merged commit dea2a03 into cockroachdb:master Sep 19, 2019
@jseldess jseldess mentioned this pull request Oct 1, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maddyblue maddyblue maddyblue approved these changes

@bdarnell bdarnell Awaiting requested review from bdarnell

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@knz @cockroach-teamcity @maddyblue @jordanlewis